fix: unable to clear contact manager field #38265
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: acd1f78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughReplaces the contact update flow with a patch-style API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI/API)
participant Server as Omnichannel API
participant Model as LivechatContacts.model
participant DB as MongoDB
participant Post as Post-update flows
rect rgba(200,220,255,0.5)
Client->>Server: POST omnichannel/contacts.update (contactId, contactManager: "")
end
rect rgba(200,255,200,0.5)
Server->>Model: patchContact(contactId, { set: {...}, unset?: ['contactManager'] })
Model->>DB: findOneAndUpdate({ _id, enabled: { $ne: false } }, { $set, $unset? })
DB-->>Model: updatedContact | null
Model-->>Server: updatedContact | null
end
rect rgba(255,230,200,0.5)
Server->>Post: trigger name/room/subscription/inquiry updates using updatedContact
Server-->>Client: HTTP 200 { success: true, contact: updatedContact }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abhinavkrin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/LivechatContacts.ts (1)
117-133: Remove the inline comment on line 122 to follow coding guidelines.Per the coding guidelines for TypeScript files, avoid code comments in implementation. The comment explaining the contactManager unsetting intent is unnecessary—the code logic is clear from the conditional check and operation.
The MongoDB behavior regarding empty
$unsetobjects is handled correctly for the driver versions in use (6.10.0+). However, the main issue is the inline comment that violates the stated guideline.
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/lib/contacts/updateContact.ts (1)
74-82: Logic is correct for enabling contactManager clearing.The use of
'contactManager' in paramscorrectly detects key presence regardless of value, enabling the model layer to handle empty strings as unset operations. The validation at lines 39-41 appropriately only triggers for truthy values.However, as per coding guidelines for TypeScript files, consider removing the inline comment on line 78. The behavior is self-documenting through the
'in'operator usage, and the model layer's handling can be documented in the model itself if needed.🔧 Suggested change
const updatedContact = await LivechatContacts.updateContact(contactId, { name, ...(emails && { emails: emails?.map((address) => ({ address })) }), ...(phones && { phones: phones?.map((phoneNumber) => ({ phoneNumber })) }), - ...('contactManager' in params && { contactManager }), // A contactManager of empty string or undefined will be unset in the model method + ...('contactManager' in params && { contactManager }), ...(channels && { channels }), ...(customFieldsToUpdate && { customFields: customFieldsToUpdate }), ...(wipeConflicts && { conflictingFields: [] }), });
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38265 +/- ##
===========================================
+ Coverage 70.76% 70.89% +0.13%
===========================================
Files 3159 3161 +2
Lines 109397 109813 +416
Branches 19657 19729 +72
===========================================
+ Hits 77416 77855 +439
+ Misses 29950 29939 -11
+ Partials 2031 2019 -12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/28 16:57 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 28 Jan 2026 16:57:22 GMT |
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
Also, might the same issue exist in resolveContactConflicts.ts for the omnichannel/contacts.conflicts endpoint? It also uses a truthy check which would prevent clearing the contactManager field.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.ts (1)
46-69: Contact-manager conflicts won’t clear when contactManager is emptied.Clearing contactManager now uses
unset, butfieldsToRemoveonly removesmanagerwhencontactManageris truthy. With an empty string, the conflict remains. Align this with the same condition used forunset.🐛 Proposed fix
- const fieldsToRemove = new Set<string>( - [ - name && 'name', - contactManager && 'manager', + const fieldsToRemove = new Set<string>( + [ + name && 'name', + ('contactManager' in params) && 'manager', ...(customFields ? Object.keys(customFields).map((key) => `customFields.${key}`) : []), ].filter((field): field is string => !!field), );
🤖 Fix all issues with AI agents
In `@apps/meteor/app/livechat/server/lib/contacts/updateContact.ts`:
- Around line 74-85: The patchContact call is including name even when
undefined, which can overwrite existing names; update the
LivechatContacts.patchContact invocation in updateContact.ts to only add name to
the set object when params.name is explicitly provided (e.g., check for
params.hasOwnProperty('name') or typeof name !== 'undefined') so that the set
payload omits name when not present; keep the other conditional spreads (emails,
phones, contactManager, channels, customFieldsToUpdate, wipeConflicts) as-is and
retain the existing unset logic for contactManager.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/livechat/contacts.ts (1)
795-836: Drop inline comments in test body.Guideline asks to avoid code comments in implementation; these can be removed without losing intent.
As per coding guidelines, avoid code comments in implementation.♻️ Proposed cleanup
- // Create a conflict await request.post(api('livechat/custom.field')).send({ token, key: 'cf1', value: '111', overwrite: true }).expect(200); await request.post(api('livechat/custom.field')).send({ token, key: 'cf1', value: '222', overwrite: false }).expect(200); - // Verify the contact has a contact manager and conflicts const contactBefore = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ contactId }).expect(200);
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/livechat/server/lib/contacts/updateContact.ts">
<violation number="1" location="apps/meteor/app/livechat/server/lib/contacts/updateContact.ts:76">
P2: Using a truthy check here skips updates when `name` is an empty string, but downstream logic still treats `name` as a change and updates rooms/subscriptions. This can leave the contact name unchanged while related data is cleared. Prefer checking for undefined so empty strings are persisted consistently.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
abhinavkrin
force-pushed
the
fix/clear-contact-manager-field
branch
from
264eaa1 to
2a05f70
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/livechat/server/lib/contacts/updateContact.spec.ts (1)
15-35: Align proxyquire stub path with the actual import specifier.
updateContact.tsimports from'./patchContact'(no extension), but the test stubs'./patchContact.ts'(with extension). The proxyquire specifiers must match exactly; a mismatch causes the stub to be ignored.✅ Proposed fix
-const { patchContact } = proxyquire.noCallThru().load('./patchContact.ts', { +const { patchContact } = proxyquire.noCallThru().load('./patchContact', { '@rocket.chat/models': modelsMock, }); ... - './patchContact.ts': { + './patchContact': { patchContact, },
🤖 Fix all issues with AI agents
In
`@apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.spec.ts`:
- Around line 18-30: The proxyquire stubs use a different module specifier than
production so the dependency isn't intercepted: change both occurrences where
proxyquire.noCallThru().load is given './patchContact.ts' to use the exact same
specifier as the real import ('./patchContact') so the stub for patchContact
(used when loading resolveContactConflicts via resolveContactConflicts)
correctly overrides the module; update the string keys passed to proxyquire for
both the standalone patchContact load and the resolveContactConflicts stub
mapping.
In `@apps/meteor/app/livechat/server/lib/contacts/updateContact.spec.ts`:
- Around line 61-69: The test passes null for contactManager but the API
contract expects a string to clear the field; update the test call to
updateContact to use an empty string ('') for contactManager (reference the test
case invoking updateContact and the assertions on
modelsMock.LivechatContacts.patchContact) so the payload matches the params type
and represents the intended “clear” value.
apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.spec.ts
Show resolved
Hide resolved
apps/meteor/app/livechat/server/lib/contacts/updateContact.spec.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Proposed changes (including videos or screenshots)
This PR fixes an issue where the Contact Manager field could not be cleared (set to empty) for a contact, either via the UI or through the API.
The PR brings changes which will allow clearing the contactManager field by broviding an empty string for the 'contactManager' field in the payload for the endpoint
omnichannel/contacts.update.Issue(s)
Steps to test or reproduce
Further comments
SUP-954
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.